Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(fixtures): fix hmac fixtures #109

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

bucko13
Copy link
Contributor

@bucko13 bucko13 commented Jan 3, 2024

Fixtures didn't match wallet configs. Some earlier change must have broken this and the fixtures weren't caught as well. With this change, all relevant tests in the caravan test suite pass (nest segwit signing still doesn't work but that's not related, it was failing w/ a different error previously though)

Screenshot 2024-01-03 at 2 28 10 PM

@dylan-thompson
Copy link

I'm not sure if this is related but for all the fixtures you updated key 0 (as displayed in caravan) is shown as key 1 when registering the wallet on my ledger and vice-versa.

Also, the tests that are failing for you work for me. Is your bitcoin testnet app up to date?

@dylan-thompson
Copy link

The only ledger test failing for me is Export mainnet xpub at m/45'/0'/4'/99'/2147483647/3/1 due to the derivation path being too long. Should we keep the test or remove it?

@bucko13
Copy link
Contributor Author

bucko13 commented Jan 3, 2024

I'm not sure if this is related but for all the fixtures you updated key 0 (as displayed in caravan) is shown as key 1 when registering the wallet on my ledger and vice-versa.

Ah, this might've been what changed the hmac. We weren't sorting the keys before and now we do to make the registrations less brittle. Might be best to change that in caravan (sort them before showing them in the table)

Also, the tests that are failing for you work for me. Is your bitcoin testnet app up to date?
Very possible it's not! Good to hear that his got fixed though. I'll try again.

@bucko13
Copy link
Contributor Author

bucko13 commented Jan 3, 2024

The only ledger test failing for me is Export mainnet xpub at m/45'/0'/4'/99'/2147483647/3/1 due to the derivation path being too long. Should we keep the test or remove it?

Hard to say. Possible they'll start allowing this eventually. Also not all devices will fail for this I don't think... Might be something to consider for the test suite, and maybe when/if we rebuild it to be more customizable and robust for these types of edge cases. Like, I think it's good to have but the test suite should support flagging tests like this.

@bucko13 bucko13 merged commit 071467a into unchained-capital:master Jan 4, 2024
3 checks passed
@bucko13 bucko13 deleted the update-fixtures branch January 4, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants